-
Notifications
You must be signed in to change notification settings - Fork 10.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update product editor CES modal to match new designs #38592
Conversation
Hi @nathanss, Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
Test Results SummaryCommit SHA: 63605db
To view the full API test report, click here. To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
Just left one suggestion and one question. Nice job!
</div> | ||
></Text> | ||
<fieldset className="woocommerce-product-mvp-feedback-modal__reason"> | ||
<legend> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you increase the margin bottom here? It is very close to the checkboxes and the margin is bigger in the design
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch! Updated from $gap-smaller
to $gap-small
.
} | ||
|
||
legend { | ||
font-size: 11px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it's a question instead of a suggestion: do we have any standards for font-sizes? 11px seems odd (literally). I see it's the same size of the design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've used it in a lot of places, but I'm not sure if there is a style guide somewhere for this. It would be really nice to create variables around this (e.g., $font-size-label
, $font-size-help
, etc). Not sure if something like this already exists on the design side. cc @jarekmorawski
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
Just left one suggestion and one question. Nice job!
Updated based on your feedback, @nathanss! Should be ready for another review. |
@joshuatf approved! There are some lint problems. Let me know when you fix them and I can approve again |
Hey @nathanss, fixed up those lint errors. This PR is ready for another review when you have time 🙇 |
Submission Review Guidelines:
Changes proposed in this Pull Request:
Updates the fields and designs of the CES modal.
Closes #38576 .
How to test the changes in this Pull Request:
Using the WooCommerce Testing Instructions Guide, include your detailed testing instructions:
localStorage.setItem( 'debug', '*tracks*' );
and also check "Preserve log"wcadmin_product_mvp_feedback
containing the email, comments, and checked reasons